Add read-only calendar from url #126862
Add read-only calendar from url #126862
Conversation
|
Hey there @allenporter, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
|
zeehio
force-pushed
the
feature/local-calendar-url
branch
from
91f436a to
3fe1b6b
Compare
|
Not sure if we can add an expandable panel to the config flow, but it would be nice to have ICS URL and time interval in a separate section that is hidden by default. |
| vol.Optional(CONF_CALENDAR_URL): TextSelector( | ||
| TextSelectorConfig(type=TextSelectorType.URL) | ||
| ), | ||
| vol.Optional(CONF_SYNC_INTERVAL): DurationSelector( |
There was a problem hiding this comment.
This needs to be removed from the config flow. The design standards for how to do updates are here https://developers.home-assistant.io/docs/integration_fetching_data/ -- This is something generic that applies to any entity and not something we want to add to the integration specific configuration flow.
The way we'll want to handle this is:
- Pick a reasonable default for how often to refresh this (not too often)
- Users can add their own automation and a schedule using an action call to
homeassistant.update_entityto refresh
There was a problem hiding this comment.
I will follow those instructions. Thank you. I have a lot to learn about coding for home assistant integrations.
Since each remote calendar entity has its own URL, I first thought I should go for Separate polling for each individual entity, and use the async_update() method for polling the data.
However I believe the async_update() method is currently called when the calendar entity needs to update the next event attribute, and I do not think we need to fetch the data that frequently. Considering this, having one coordinator class for each entity seems to make more sense. Would you agree? Suggestions are welcome.
There was a problem hiding this comment.
Great thanks. Very happy to have your contribution, and while there is a learning curve it is quite rewarding to contribute to home assistant so really happy to help here. You have a great start already and reviewers are happy to point out best practices that may not be easy to find.
Yeah doing the polling in each entity is fine.
OK so the calendar entity is a bit special, and so i can explain here how it works. Consider here a scenario for Google calendar: Say every 15 minutes you refresh the cloud calendar, but then a new event shows up. It starts in 5 minutes, so you also need to handle the state transition from "off" to "on". How this works is the calendar base class will handle this for you. You'd just refresh the calendar event every 15 minutes and the base class will set an additional timer based on event for 5 minutes and it will trigger another state change.
See
for details on how this works.So yeah doing it per entity as you said is fine.
| @@ -35,6 +61,31 @@ async def async_step_user( | |||
| key = slugify(user_input[CONF_CALENDAR_NAME]) | |||
| self._async_abort_entries_match({CONF_STORAGE_KEY: key}) | |||
| user_input[CONF_STORAGE_KEY] = key | |||
| if url := user_input.get(CONF_CALENDAR_URL): | |||
There was a problem hiding this comment.
Increase test coverage for this file.
There was a problem hiding this comment.
Sure, tests and documentation are a must for the merge. I was coding this from my phone, on my live home assistant instance and I did not have access to tools to run the testsuite.
I will add the tests and a documentation pull request as well
| try: | ||
| vol.Schema(vol.Url())(url) | ||
| except vol.Invalid: | ||
| return self.async_show_form( |
There was a problem hiding this comment.
Restructure this method to have a single show form call, not one per exception.
| self._etag = None | ||
| self._update_interval = update_interval | ||
|
|
||
| async def _fetch_calendar_and_update(self): |
There was a problem hiding this comment.
I imagined this working more like a sync where we fetch the url, writing down to the local filesystem store, but from there the local calendar integration works like normal and is able to serve from the local filesystem store. This means it continues to work offline and truly is still a local calendar. I think it could fetch and write to the store, then work like normal (but still readonly).
I think this could be a good way to think about business logic separation for the "Store" class and fetching/reading/writing. (Right now there is a "store" for a remote calendar entity and it is ambiguous what its for so something needs to be addressed here)
There was a problem hiding this comment.
I understand your proposal and I agree with it. It makes a lot of sense. I will implement the changes and tests and ask for a second review.
Thank you very much for your time
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
One thing i wanted to point out is there is another draft PR in parallel adding ability to upload an ICS file. I may take over that PR and try to get it moving as I think adding both of these features for the next release will be a really cool package. We'll just have to figure out how we want the combined config flow to work: Maybe we end up making a radio button with two separate flows for uploading vs importing from a url? |
I updated #117955 to support a radio button with option to either (1) create empty or (2) upload the ICS file. We could add (3) to add a new step to import from a URL |
Great to know. Since it was just merged it's much easier for me to rebase my PR and keep working from there. :-) |
allenporter
added
the
noteworthy
|
Talking to one of the other core maintainers, there is a preference to do this in a separate integration "remote calendar". It can copy code from the local calendar component (without the writeable actions) and can share most of the same code (similar to how todo is also using all the same code). Sound reasonable? Let me know if you need any help with this as i think it'll be nice to have this in the next release and i'm happy to do reviews for it/help how i can. |
With respect to doing this on a separate integration it makes perfect sense to me as well. With respect to timings I can't make it to the October release. My free time is limited and not regularly spaced, so I can't commit to having it done by some date, unfortunately. On the bright side, if you (or anyone else) have the time to push this forward and a clear idea of what needs to get done I will be super happy to step aside and help with code review as much as I can. I am happy as long as eventually we see this feature in. I will post a message here before I start any kind of further work on this, so please feel free to do the same and take over if you can. |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Proposed change
Allow to provide a URL pointing to an ics file when creating a local calendar. If the URL is given, the calendar becomes read-only and shows events from the ics provided by the URL. The integration will refresh the URL once a day by default, although the update interval is customizable.
This is useful for users who live in towns who publish local holidays or garbage collection information in ics files, if they want to have such information in home assistant.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: